Skip to content

Conversation

@xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Nov 5, 2025

… true

Make sure this field, which has been moved from SpanPolicy to ValueStoragePolicy, is read for the DisableValueBlocks option in the sst writer.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz force-pushed the fix-span-policy-disable-value-blocks branch 5 times, most recently from ed1466e to 9f5e0a0 Compare November 5, 2025 20:09
@xinhaoz xinhaoz marked this pull request as ready for review November 5, 2025 20:10
@xinhaoz xinhaoz requested a review from a team as a code owner November 5, 2025 20:10
@xinhaoz xinhaoz requested a review from RaduBerinde November 5, 2025 20:10
@xinhaoz xinhaoz force-pushed the fix-span-policy-disable-value-blocks branch 2 times, most recently from 8859026 to 10c0e65 Compare November 5, 2025 20:16
Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions


options.go line 1312 at r1 (raw file):

// KVs, overriding global policies. Values can be configured to be stored in-place,
// in value blocks, or in blob files.
type ValueStoragePolicyAdjustment struct {

I think we can simplify this to just a set of fields.

type ValueStoragePolicyAdjustment struct {
   DisableBlobSeparation bool
   OverrideBlobSeparationMinimumSize int
   DisableSeparationBySuffix bool
}

Zero value means no adjustment.


options.go line 1320 at r1 (raw file):

	// keys (where the smallest suffix is the latest version), but should be
	// disabled for keys where the suffix does not correspond to a version.
	DisableSeparationBySuffix bool

It would be good to document the of cases of where values can go, with or without this flag, for the three possibilities:

  • most recent suffix with value >= MinimumSize
  • older suffix with value >= MinimumSize
  • older suffix with value < MinimumSize

compaction.go line 3491 at r1 (raw file):

			// aggressive value separation policy for this output.
			vSep.SetNextOutputConfig(valsep.ValueSeparationOutputConfig{
				MinimumSize:                    spanPolicy.ValueStoragePolicy.MinimumSize,

[nit] maybe we should rename MinimumSizeForBlob or similar

@xinhaoz xinhaoz force-pushed the fix-span-policy-disable-value-blocks branch from 10c0e65 to e6f9e82 Compare November 5, 2025 22:31
Copy link
Member Author

@xinhaoz xinhaoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @RaduBerinde)


options.go line 1312 at r1 (raw file):

Previously, RaduBerinde wrote…

I think we can simplify this to just a set of fields.

type ValueStoragePolicyAdjustment struct {
   DisableBlobSeparation bool
   OverrideBlobSeparationMinimumSize int
   DisableSeparationBySuffix bool
}

Zero value means no adjustment.

Done.


options.go line 1320 at r1 (raw file):

Previously, RaduBerinde wrote…

It would be good to document the of cases of where values can go, with or without this flag, for the three possibilities:

  • most recent suffix with value >= MinimumSize
  • older suffix with value >= MinimumSize
  • older suffix with value < MinimumSize

Done.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @xinhaoz)


options.go line 1330 at r2 (raw file):

	// - If blob separation is disabled, the value will be written to a value
	// block within the sstable.
	// - Otherwise, the KV will be subject to regular value separation rules.

[nit] There is no "otherwise" (separation is either enabled or disabled)

… true

Make sure this field, which has been moved from SpanPolicy to
ValueStoragePolicy, is read for the DisableValueBlocks option in the sst writer.
@xinhaoz xinhaoz force-pushed the fix-span-policy-disable-value-blocks branch from e6f9e82 to 26a4f68 Compare November 6, 2025 14:39
@xinhaoz
Copy link
Member Author

xinhaoz commented Nov 6, 2025

TFTR!

@xinhaoz xinhaoz merged commit b0c3d6c into cockroachdb:master Nov 6, 2025
9 checks passed
@xinhaoz xinhaoz deleted the fix-span-policy-disable-value-blocks branch November 6, 2025 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants